Cleanup some code under FEATURE_STUBS_AS_IL #104731

huoyaoyuan · 2024-07-11T13:48:19Z

There's no actually no IL stubs under this feature switch now, only asm stubs.

This PR addresses the simple cases. On all platforms except Windows x86, single cast delegate is written in invariant assembly code.

Remaining code under the feature switch is COM call stub, which is more complicated and is better separated out.

src/coreclr/vm/i386/asmhelpers.asm

src/coreclr/vm/jitinterface.h

src/coreclr/vm/stubmgr.cpp

jkotas · 2024-07-17T14:49:15Z

Code quality

If this helps with code quality, could you please make the same change in

runtime/src/coreclr/tools/Common/TypeSystem/IL/Stubs/DelegateMethodILEmitter.cs

Lines 59 to 60 in 3f607bd

    
           codeStream.EmitLdArg(0); 
        
           codeStream.Emit(ILOpcode.ldfld, emit.NewToken(functionPointerField.InstantiateAsOpen()));

as well? It is naot equivalent of this stub.

src/coreclr/vm/comdelegate.cpp

huoyaoyuan · 2024-07-17T15:53:50Z

The same tests in DI are still failing. Can it be related to the signature instantiation in ILC?

runtime/src/coreclr/tools/Common/TypeSystem/IL/Stubs/DelegateMethodILEmitter.cs

Lines 63 to 81 in 3f607bd

    
           if (method.OwningType.HasInstantiation) 
        
           { 
        
               // If the owning type is generic, the signature will contain T's and U's. 
        
               // We need !0's and !1's. 
        
               TypeDesc[] typesToReplace = new TypeDesc[method.OwningType.Instantiation.Length]; 
        
               TypeDesc[] replacementTypes = new TypeDesc[typesToReplace.Length]; 
        
               for (int i = 0; i < typesToReplace.Length; i++) 
        
               { 
        
                   typesToReplace[i] = method.OwningType.Instantiation[i]; 
        
                   replacementTypes[i] = context.GetSignatureVariable(i, method: false); 
        
               } 
        
               TypeDesc[] parameters = new TypeDesc[method.Signature.Length]; 
        
               for (int i = 0; i < parameters.Length; i++) 
        
               { 
        
                   parameters[i] = method.Signature[i].ReplaceTypesInConstructionOfType(typesToReplace, replacementTypes); 
        
               } 
        
               TypeDesc returnType = method.Signature.ReturnType.ReplaceTypesInConstructionOfType(typesToReplace, replacementTypes); 
        
               signature = new MethodSignature(signature.Flags, signature.GenericParameterCount, returnType, parameters); 
        
           }

Is it also required for coreclr?

jkotas · 2024-07-18T04:25:29Z

The same tests in DI are still failing. Can it be related to the signature instantiation in ILC?

We should not need a special signature instantiation like that in CoreCLR. I suspect that it is related to a special delegate calling convention used for delegate Invocation in some cases. The delegate target cannot be called via pointer - it must be called as Delegate.Invoke method so that the special calling convention kicks in.

Do you know if the Invoke method is used in the DI on a performance critical path?

This is proving to be much harder than what I thought it is going to be.

huoyaoyuan · 2024-08-23T12:49:49Z

I can't see more open issues for this, and main should be open for 10.0 now.

Given the stub is not in hot path, it's should be a viable solution to call a trace helper like the multi-cast stub.

mangod9 · 2024-09-16T14:47:51Z

@jkotas @noahfalk could you please take another look so we can merge?

noahfalk · 2024-09-17T02:39:05Z

@jkotas @noahfalk could you please take another look so we can merge?

My only involvement so far was answering a question about singlecast delegate stepping for the debugger which was resolved out-of-scope for now. If you need me to I could spend time and dig into this much deeper but I am assuming that @jkotas already has the context so its more efficient for him to continue. Just let me know.

src/coreclr/debug/ee/frameinfo.cpp

src/coreclr/vm/amd64/cgencpu.h

src/coreclr/vm/class.h

src/coreclr/vm/comdelegate.cpp

src/coreclr/vm/stubmgr.cpp

Co-authored-by: Jan Kotas <[email protected]>

src/coreclr/vm/dllimport.h

src/coreclr/vm/stublink.h

src/coreclr/vm/method.hpp

jkotas · 2024-09-19T06:43:24Z

I have pushed a commit with a small change to handle the new IL stub in stub manager. The manual debugger test from #104731 (comment) works fine now.

jkotas

LGTM. Thank you!

jkotas · 2024-09-19T15:48:39Z

Remaining code under the feature switch is COM call stub, which is more complicated and is better separated out.

Do you plan to work on cleaning up the remaining code under the feature switch?

huoyaoyuan · 2024-09-19T16:23:37Z

Do you plan to work on cleaning up the remaining code under the feature switch?

Aaron suggested me to avoid converting COM interop related code because the effort needed for reviewing. I'm already busy now until October.

If you can confirm the review effort on behalf of interop team, I can start to investigate this when I finish urgent tasks.

jkotas · 2024-09-20T06:13:04Z

I gave it a shot in #108048.

huoyaoyuan added 4 commits July 11, 2024 21:03

Use direct SinglecastDelegateInvokeStub on windows x86

c06cc3f

Cleanup ifdefs

4845202

EmitDebugBreak is unused

faf25e7

endif format

09fcf2b

dotnet-issue-labeler bot added the area-VM-coreclr label Jul 11, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 11, 2024

huoyaoyuan added 2 commits July 11, 2024 21:52

Restore comment

19057f9

Fix JIT_InternalThrowFromHelper reference

157325b

This was referenced Jul 11, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

The job running on agent NetCore-Public ran longer than the maximum time #104044

Closed

jkotas changed the title ~~Cleanup some code under FEATURE_STUBS_AS_AL~~ Cleanup some code under FEATURE_STUBS_AS_IL Jul 12, 2024

jkotas reviewed Jul 12, 2024

View reviewed changes

src/coreclr/vm/i386/asmhelpers.asm Outdated Show resolved Hide resolved

huoyaoyuan added 2 commits July 16, 2024 00:03

Use IL stub for Delegate.Invoke

c98be47

Remove SinglecastDelegateInvokeStub in asm

72fd2d6

build-analysis bot mentioned this pull request Jul 15, 2024

Test failure WindowsAlternateDataStreamOverwrite #83659

Open

huoyaoyuan added 2 commits July 17, 2024 19:24

Fix method name and argument count

e60b3f1

Code quality

7f117aa

jkotas reviewed Jul 17, 2024

View reviewed changes

src/coreclr/vm/jitinterface.h Outdated Show resolved Hide resolved

jkotas reviewed Jul 17, 2024

View reviewed changes

src/coreclr/vm/stubmgr.cpp Outdated Show resolved Hide resolved

jkotas reviewed Jul 17, 2024

View reviewed changes

src/coreclr/vm/stubmgr.cpp Outdated Show resolved Hide resolved

Delete JIT_InternalThrow and CorInfoException

cd648fa

huoyaoyuan commented Jul 17, 2024

View reviewed changes

src/coreclr/vm/comdelegate.cpp Outdated Show resolved Hide resolved

huoyaoyuan mentioned this pull request Jul 17, 2024

JIT unnecessary stack load/stores for tail jump with stack args involved #105031

Open

Apply the same codegen to ilc

b078f3a

This was referenced Jul 17, 2024

System.IO.Net5Compat.Tests and System.IO.Tests suddenly exiting with error 137 #100558

Open

SIGKILL (OOM?) while running LibraryImportGenerator.Tests w/o actionable log messages or artifacts dotnet/dnceng#2496

Open

huoyaoyuan mentioned this pull request Jul 18, 2024

Some delegate invoke thunks fail to compile with where T : allows ref struct delegates #105029

Closed

jkotas reviewed Sep 18, 2024

View reviewed changes

jkotas mentioned this pull request Sep 18, 2024

Cleanup delegate-related code #107966

Merged

Apply suggestions from code review

8992882

Co-authored-by: Jan Kotas <[email protected]>

build-analysis bot mentioned this pull request Sep 18, 2024

restarted. Azure DevOps can't recover from restarts. dotnet/dnceng#3879

Open

3 tasks

huoyaoyuan added 3 commits September 18, 2024 20:02

Merge branch 'main' into stubs-as-il

e06998c

Return Stub from GetInvokeMethodStub

c221eed

Delete NEWSTUB_FL_MULTICAST and dependents

1c640f8

jkotas reviewed Sep 18, 2024

View reviewed changes

src/coreclr/vm/dllimport.h Outdated Show resolved Hide resolved

jkotas reviewed Sep 18, 2024

View reviewed changes

src/coreclr/vm/dllimport.h Outdated Show resolved Hide resolved

jkotas reviewed Sep 18, 2024

View reviewed changes

src/coreclr/vm/stublink.h Outdated Show resolved Hide resolved

jkotas reviewed Sep 18, 2024

View reviewed changes

src/coreclr/vm/stublink.h Outdated Show resolved Hide resolved

huoyaoyuan added 4 commits September 19, 2024 10:59

Rename

e39afa1

Renumber

eebbc01

Merge branch 'main'

c2cf515

Format

bc1a766

jkotas reviewed Sep 19, 2024

View reviewed changes

src/coreclr/vm/method.hpp Outdated Show resolved Hide resolved

jkotas added 2 commits September 18, 2024 22:15

Update src/coreclr/vm/method.hpp

ee674a3

Handle new stub in ILStubManager

fdd83eb

jkotas approved these changes Sep 19, 2024

View reviewed changes

build-analysis bot mentioned this pull request Sep 19, 2024

Test failure: System.Security.Cryptography.X509Certificates.Tests.PfxIterationCountTests_X509Certificate2.Import_IterationCountLimitExceeded_ThrowsInAllottedTime #105609

Open

jkotas merged commit cc17a46 into dotnet:main Sep 19, 2024
92 of 96 checks passed

huoyaoyuan deleted the stubs-as-il branch September 19, 2024 16:18

BruceForstall mentioned this pull request Sep 20, 2024

Test build failure: "coreclr Common Pri1 Test Build AnyOS AnyCPU checked" CI job fails #108076

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup some code under FEATURE_STUBS_AS_IL #104731

Cleanup some code under FEATURE_STUBS_AS_IL #104731

huoyaoyuan commented Jul 11, 2024 •

edited

Loading

jkotas commented Jul 17, 2024

huoyaoyuan commented Jul 17, 2024

jkotas commented Jul 18, 2024 •

edited

Loading

huoyaoyuan commented Aug 23, 2024

mangod9 commented Sep 16, 2024

noahfalk commented Sep 17, 2024

jkotas commented Sep 19, 2024

jkotas left a comment

jkotas commented Sep 19, 2024

huoyaoyuan commented Sep 19, 2024

jkotas commented Sep 20, 2024

Cleanup some code under FEATURE_STUBS_AS_IL #104731

Cleanup some code under FEATURE_STUBS_AS_IL #104731

Conversation

huoyaoyuan commented Jul 11, 2024 • edited Loading

jkotas commented Jul 17, 2024

huoyaoyuan commented Jul 17, 2024

jkotas commented Jul 18, 2024 • edited Loading

huoyaoyuan commented Aug 23, 2024

mangod9 commented Sep 16, 2024

noahfalk commented Sep 17, 2024

jkotas commented Sep 19, 2024

jkotas left a comment

Choose a reason for hiding this comment

jkotas commented Sep 19, 2024

huoyaoyuan commented Sep 19, 2024

jkotas commented Sep 20, 2024

huoyaoyuan commented Jul 11, 2024 •

edited

Loading

jkotas commented Jul 18, 2024 •

edited

Loading